Passivation configured in language supports via the discovery protocol #486

ralphlaude · 2020-11-13T13:35:11Z

Resolves #298

Passivation timeouts configured in language supports via the discovery protocol. The first step is the java-support and then node-support.

In the java-support all entity annotations are extended @XXXEntity (passivationTimeout = 10), the service class io.cloudstate.javasupport.Service is also extented with passivationTimeout and the grpc entity in the entity.proto.

# Conflicts: # proxy/core/src/main/resources/in-memory.conf # proxy/core/src/main/scala/io/cloudstate/proxy/EntityDiscoveryManager.scala # proxy/jdbc/src/main/resources/jdbc-common.conf

ralphlaude · 2020-11-13T13:39:07Z

@pvlugter what do you think about the option?

sleipnir

Guy thinks that this option should be passed via registryXXXXEntity methods and not in the entity's annotation. I think this pollutes the entity a little.

sleipnir · 2020-11-13T14:43:09Z

java-support/src/main/java/io/cloudstate/javasupport/crdt/CrdtEntity.java

+   * use default from configuration file. Any positive value means the entity is passivated after
+   * the idle time.
+   */
+  int passivationTimeout() default 0;


Hello Guy I didn't like this option for me it should be done at the Entity registration stage and not at the entity itself.

Hi Adriano, it is also fine for me and it is easier. Why do you like the option over the registration?

Great! Thanks

why do you like this option?

Why would 0 not be a valid value? And therefore passivation disabled at all?

and it only sends a timeout if it's been configured in the user function, rather than relying on negative numbers. And the protocol could have a duration message, with time units, to make things complete.

I agree to change the protocol now and a soon as possible for such changes. I also agree on not depending on magic values.

I think I'd like to have options for expression the following things - perhaps not immediately, but we should choose a mechanism and protocol that allows us to add support for them in future if needed:

A specific passivation timeout, with at least millisecond resolution.

Default - note that I think it's fine for the default to come from the support library, I don't see any reason why the proxy needs to be in control of the default.

No passivation timeout - entities should never passivate unless rebalanced.

Immediate passivation, this effectively means the state is reloaded for each command.

Some way to indicate the proxy should apply smarter, more dynamic passivation strategies, for example strategies that respond to resource constraints, or perhaps auto-tune themselves based on cardinality and/or hot spot metrics.

So, basically, I think it's worth now choosing a strategy in the APIs and protocol that will allow us to add some of the features above without breaking the protocol or APIs.

Good idea to future-proof the protocol and APIs now for other passivation options.

Really good idea for passivation options.
More generally we should have a strategy for adding other future options (not only passivation) without breaking the protocol or the APIs.

After some more discussion:

passivation timeout in protocol can just be milliseconds, we don't expect more fine-grained than that

language supports can use duration APIs or similar as appropriate, convert to milliseconds

protocol should support expanding to other passivation strategies, where initially it could just be timeout, ideally we can add never or immediate or other strategies in a protocol-compatible way

protocol should support strategies explicitly, such as timeout, never or immediate, and not use magic values

…entity options to pass the configured at registration time

ralphlaude · 2020-11-24T08:49:47Z

The discovery process, the proxy and the API have been extended to support passivation strategy and also that new passivation strategy (never, immedaite and perhaps resource specifics strategies) can be added without breaking the API. The only supported passivation strategy right now is timeout. Passivation strategy could be as complex as possible to configure and this does not fit within annotations. So the approach to pass configuration parameter at registration time is a better fit. There is EntityOptions for configuring passivation strategy and other aspects if needed.
The language support should deal with default passivation timeout and more generally fallback.
All entities must support a passavation strategy and right now it is timeout. The entity.proto file has been modified like this:

message Entity {
    ...

    // The passivation strategy for the entity.
    EntityPassivationStrategy passivation_strategy = 4;
}

// A passivation strategy for the entity which is sent to the proxy.
message EntityPassivationStrategy {
    oneof strategy {
	// the timeout passivation strategy.
        TimeoutPassivationStrategy timeout = 1;
        ImmediatePassivationStrategy immediate = 2; // this strategy can be added later on
        NeverPassivationStrategy never = 3; // this strategy can also be added later on
        // more strategies can be added here
    }
}

// A passivation strategy based on a timeout. The idle timeout after which an entity is passivated.
message TimeoutPassivationStrategy {
    // The timeout in millis
    int64 timeout = 1;
}

// A passivation strategy which passivates the entity after a command is handled.
message ImmediatePassivationStrategy {}

// A passivation strategy which never passivates the entity.
message NeverPassivationStrategy {}

For the java support it will be like at the end:

// passivation stratgey timeout with default passivation timeout set which is sent with entity spec to proxy
@EventSourcedEntity(persistenceId = "something")

// no extra options specified, passivation stratgey timeout with default timeout sent with entity spec to proxy
.registerEventSourcedEntity(
  SomeEntity.class,
  SomeProtocol.getDescriptor().findServiceByName("SomeService"),
  SomeProtocol.getDescriptor())

// options specified with passivation stratgey timeout, which will be sent with entity spec to proxy
.registerEventSourcedEntity(
  SomeEntity.class,
  SomeProtocol.getDescriptor().findServiceByName("SomeService"),
  EventSourcedEntityOptions.defaults().withPassivationStrategy(PassivationStrategy.timeout(Duration.ofSeconds(10)))
  SomeProtocol.getDescriptor())

Only the java support is implemented and all others language supports (Nodejs, GO, Kotlin, Python, Spring and so on) should be extended before this can be merged.
Wo can help with other language supports (Nodejs, GO, Kotlin, Python, Spring and so on)?

marcellanz · 2020-11-24T10:59:34Z

Thanks @ralphlaude :) For Go for sure I can help.
If needed we could work based on a dedicated branch so that maintained language supports can be aligned. As the new field EntityPassivationStrategy protocol-wise is an addition, entity discovery could probably even work without the support on user languages?

ralphlaude · 2020-11-24T11:33:05Z

@marcellanz thanks for Go.
Yes we can think how to work on a dedicated branch to be aligned.
With the addition of the new field EntityPassivationStrategy in the protocol the discovery is broken without the support on user language. It can be changed if we want it.

sleipnir · 2020-11-24T12:05:46Z

Hi @ralphlaude , thanks for another one!
I can help with the other languages mentioned but generally as Kotlin and Springboot depend on the java-support jar I choose to wait for the release of an official release with the functionality, I do this to not have to build the jars locally. I sincerely wish there were more releases to help me with this, but I may have to follow the branches of the main repository with equivalent branches in the languages, I don't know, I find this approach very laborious, but this should be discussed in another forum.

marcellanz · 2020-11-24T12:17:22Z

@sleipnir I think, it should be possible or better said desirable for language supports to evolve before the core project releases. For non-JVM supports it is already manageable with tck- and proxy images available from master. For JVM support perhaps snapshot releases help? But as you said, could be discussed elsewhere.

sleipnir · 2020-11-24T12:56:15Z

For JVM support perhaps snapshot releases help? ...

Yes! But they are also often not available.

pvlugter · 2020-11-24T22:23:41Z

For JVM support perhaps snapshot releases help? ...

Yes! But they are also often not available.

It's easy to publish a versioned snapshot for java support. Just ask if you need one :)

pvlugter · 2020-11-24T22:27:21Z

With the addition of the new field EntityPassivationStrategy in the protocol the discovery is broken without the support on user language. It can be changed if we want it.

Since it's an additional field, the protocol can be compatible with older versions. In the proxy, instead of throwing exceptions if the language support doesn't define a passivation strategy, we can use the proxy-side default.

I think it will be better to have this compatible and use the proxy default, rather than require all language supports to be updated straight away. There's also an older issue on configurable entity passivation (#111) which describes having the passivation strategy defined in the Kubernetes CRD or ConfigMap and injected by the operator — which would allow the proxy-side default to be configured, if the language support doesn't define it.

ralphlaude · 2020-11-25T08:55:44Z

Since it's an additional field, the protocol can be compatible with older versions. In the proxy, instead of throwing exceptions if the language support doesn't define a passivation strategy, we can use the proxy-side default.

I think it will be better to have this compatible and use the proxy default, rather than require all language supports to be updated straight away. There's also an older issue on configurable entity passivation (#111) which describes having the passivation strategy defined in the Kubernetes CRD or ConfigMap and injected by the operator — which would allow the proxy-side default to be configured, if the language support doesn't define it.

The proxy-default for passivation strategy will be introduced again to allow the compatibility so we can move forward. I think it would better in the middle term to have a central place where to configure things like passivation strategy. Now we have two places where passivation strategy is configured. I think the better place for that is the language support.

…e default passivation strategy timeout with those configured in the language support

ralphlaude · 2020-11-26T07:02:50Z

Everything is done here. I am not sure if we want to extends the nodejs support here or do it in another issue. If yes, I will create issues for nodejs support and another language support (GO, Spring, Kotlin, Python and others).
@sleipnir, @marcellanz what do you think?

marcellanz · 2020-11-26T08:45:50Z

@ralphlaude I think for changes on the protocol one implementation for a "main" user support language should be enough. With "main" this might be JS or Java I think.

As the protocol changed more often lately, it reveals how we might work with such changes in general. With changes worked on concurrently, having the proxy and TCK in the right state for development leads to a bit of coordination.
I think we don't need overly complex rules for that like, as an example, opening issues for all support languages. Perhaps the TCK can help to guide where changes are pending for other support languages. This spans a matrix of combinations for proxy, TCK and support language implementations. The current situation already shows this, with currently a few languages aligned with master, and others partly up to date.

This said, I think support languages should aim to get protocol updates by themselves. If the protocol changes by a PR like this one, the changing PR a) should provide a TCK test or if not applicable, b) a comment in the spec about semantics changed in the protocol.

The TCK Verification here https://observablehq.com/@marcellanz/cloudstate-tck-verification displays the current situation also in an automatic way, with pending TCK tests visible.

WDYT?
CC/ @sleipnir @pvlugter

sleipnir · 2020-11-26T11:49:15Z

@ralphlaude I agree with @marcellanz but I add that the comment on the semantics desired by the protocol must be mandatory in PR. This avoids that developers have to have a long journey of understanding evaluating codes and contracts where a comment would be enough to make the intention clear.

ralphlaude · 2020-11-26T13:48:40Z

@marcellanz the proposal is fine for me. It would be nice to have support for nodejs. I will comment the spec about the new semantic.

ralphlaude · 2020-11-27T09:51:55Z

@pvlugter, @jroper I need support here for nodejs. Please take a look.

pvlugter

Looking good. Added a few review comments — would be good to not load the config in the Timeout static object in particular, but have the default come from the top-level config.

java-support/src/main/scala/io/cloudstate/javasupport/impl/PassivationStrategies.scala

protocols/protocol/cloudstate/entity.proto

proxy/core/src/main/scala/io/cloudstate/proxy/crdt/CrdtSupportFactory.scala

proxy/core/src/main/scala/io/cloudstate/proxy/eventsourced/EventSourcedSupportFactory.scala

pvlugter · 2020-11-29T23:40:34Z

Yes, would be good to add passivation timeouts for node support. Node support is behind the Java support in a few areas now: applying events immediately (and any failure handling for this) #375, support for value entities, new TCK implementation, and this. Doesn't need to be added to this PR, let's create an issue to track.

pvlugter · 2020-11-29T23:43:19Z

Could be useful to have passivation timeouts as part of the TCK for testing.

… creating default time out passivation strategy. fixed typos and string interpolation.

ralphlaude · 2020-11-30T20:33:15Z

Could be useful to have passivation timeouts as part of the TCK for testing.

@pvlugter Could you please be more specific?

pvlugter · 2020-11-30T22:13:59Z

java-support/src/main/scala/io/cloudstate/javasupport/CloudStateRunner.scala

-      val conf = ConfigFactory.load()
-      conf.getConfig("cloudstate.system").withFallback(conf)
-    }), services.asScala.toMap)
+    this(ActorSystem("StatefulService", CloudStateConfigHolder.defaultConfiguration()), services.asScala.toMap)


It's the other constructor, that takes a config explicitly, that should be supported. The config gets passed to the actor system. It should also be the config that gets passed through to other places (could be retrieved from the actor system). Might be more obvious if there's a test that sets the passivation timeout in config?

@pvlugter I hope my solution does the job and I did not find a better way.

pvlugter · 2020-11-30T22:19:42Z

Could be useful to have passivation timeouts as part of the TCK for testing.

@pvlugter Could you please be more specific?

Since passivation strategy is part of the protocol, it would be useful to test it in the TCK. So testing that language supports can configure this, and ideally also testing that the proxy applies the timeout and passivates the entity. For the TCK, it would be fine to have an additional entity defined which is just for testing this, since the passivation timeout is on entity discovery.

…or passivation timeout

# Conflicts: # java-support/tck/src/main/java/io/cloudstate/javasupport/tck/JavaSupportTck.java # proxy/core/src/main/scala/io/cloudstate/proxy/UserFunctionTypeSupport.scala # proxy/core/src/main/scala/io/cloudstate/proxy/eventsourced/EventSourcedSupportFactory.scala # tck/src/main/scala/io/cloudstate/tck/CloudStateTCK.scala

pvlugter

Okay, I can always follow up with something that doesn't use a global static config

tck/src/main/scala/io/cloudstate/tck/CloudStateTCK.scala

proxy/core/src/main/resources/reference.conf

…ement the passivation strategy

ralphlaude · 2020-12-09T22:39:26Z

@pvlugter ping! you can follow up.

pvlugter · 2020-12-17T00:33:43Z

I'll just close and reopen this PR to see if we have Travis CI working again.

pvlugter

👍🏼 Good to see the global config part updated.

I'll merge this now, and include in the TCK reorganisation.

ralphlaude added 4 commits November 7, 2020 18:41

allow the proxy to configure passivation for entity type.

efdf843

add settings for entity type

b711740

Merge branch 'master' into passivation-per-entity-type

f01a206

# Conflicts: # proxy/core/src/main/resources/in-memory.conf # proxy/core/src/main/scala/io/cloudstate/proxy/EntityDiscoveryManager.scala # proxy/jdbc/src/main/resources/jdbc-common.conf

Add passivation timeout in the java-support and the entity discovery

62c461c

sleipnir reviewed Nov 13, 2020

View reviewed changes

extend the discovery process to support passivation strategy and use …

10e056a

…entity options to pass the configured at registration time

ralphlaude added 3 commits November 25, 2020 15:05

restore default passivation strategy timeout in the proxy. Combine th…

296cbe3

…e default passivation strategy timeout with those configured in the language support

removed env-vars for passivation timeout

1079f94

removed unused imports

549bd1b

add specification description for the passivation strategy

6225e25

pvlugter reviewed Nov 29, 2020

View reviewed changes

pvlugter mentioned this pull request Nov 29, 2020

TCK – verify entity passivation #359

Closed

2 tasks

marcellanz mentioned this pull request Nov 29, 2020

Passivation configuration by the discovery protocol cloudstateio/go-support#59

Closed

add holder for the default config and use in CloudStateRunner and for…

982044b

… creating default time out passivation strategy. fixed typos and string interpolation.

pvlugter reviewed Nov 30, 2020

View reviewed changes

ralphlaude added 3 commits December 6, 2020 14:37

change config holder to save and pass down the config. add tck test f…

7cc33b8

…or passivation timeout

merged master

0d84bf2

pvlugter reviewed Dec 7, 2020

View reviewed changes

tck/src/main/scala/io/cloudstate/tck/CloudStateTCK.scala Outdated Show resolved Hide resolved

tck/src/main/scala/io/cloudstate/tck/CloudStateTCK.scala Outdated Show resolved Hide resolved

increase passivation timeout for CI tests

b11db95

marcellanz reviewed Dec 7, 2020

View reviewed changes

tck/src/main/scala/io/cloudstate/tck/CloudStateTCK.scala Outdated Show resolved Hide resolved

marcellanz reviewed Dec 7, 2020

View reviewed changes

proxy/core/src/main/resources/reference.conf Outdated Show resolved Hide resolved

ralphlaude added 2 commits December 7, 2020 18:25

let the TCK tests failed for language support which does not yet impl…

937ec55

…ement the passivation strategy

test passivation only in the TCK model

1e6cdb5

remove global config for accessing configured passivation timeout

cc7ba5a

pvlugter closed this Dec 17, 2020

pvlugter reopened this Dec 17, 2020

pvlugter approved these changes Dec 17, 2020

View reviewed changes

pvlugter merged commit 4904645 into cloudstateio:master Dec 17, 2020

Passivation configured in language supports via the discovery protocol #486

Passivation configured in language supports via the discovery protocol #486

Conversation

ralphlaude commented Nov 13, 2020

ralphlaude commented Nov 13, 2020

sleipnir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ralphlaude Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ralphlaude commented Nov 24, 2020

marcellanz commented Nov 24, 2020

ralphlaude commented Nov 24, 2020

sleipnir commented Nov 24, 2020

marcellanz commented Nov 24, 2020

sleipnir commented Nov 24, 2020 • edited Loading

pvlugter commented Nov 24, 2020

pvlugter commented Nov 24, 2020 • edited Loading

ralphlaude commented Nov 25, 2020

ralphlaude commented Nov 26, 2020

marcellanz commented Nov 26, 2020 • edited Loading

sleipnir commented Nov 26, 2020

ralphlaude commented Nov 26, 2020

ralphlaude commented Nov 27, 2020

pvlugter left a comment

Choose a reason for hiding this comment

pvlugter commented Nov 29, 2020

pvlugter commented Nov 29, 2020

ralphlaude commented Nov 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvlugter commented Nov 30, 2020

pvlugter left a comment

Choose a reason for hiding this comment

ralphlaude commented Dec 9, 2020

pvlugter commented Dec 17, 2020

pvlugter left a comment

Choose a reason for hiding this comment

ralphlaude Nov 13, 2020 •

edited

Loading

sleipnir commented Nov 24, 2020 •

edited

Loading

pvlugter commented Nov 24, 2020 •

edited

Loading

marcellanz commented Nov 26, 2020 •

edited

Loading